Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add functionality to hide ingestion enabled flag from --help output #4955

Merged
merged 19 commits into from
Jul 24, 2023
Merged

Add functionality to hide ingestion enabled flag from --help output #4955

merged 19 commits into from
Jul 24, 2023

Conversation

aditya1702
Copy link
Contributor

@aditya1702 aditya1702 commented Jul 11, 2023 •
edited
Loading

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Closes #4906

  • Add a new Hidden config option which makes it easy to hide flags from our --help output.
  • Hide the flag: --exp-enable-ingestion-filtering since it is now enabled by default.
  • Give a DEPRECATION warning if a user still specifies the flag.
  • Integration tests to test the change.

Why

Ingestion filtering is now always enabled and does not require user specified flag.

Known limitations

[TODO or N/A]

@aditya1702 aditya1702 self-assigned this Jul 11, 2023
@aditya1702 aditya1702 linked an issue Jul 11, 2023 that may be closed by this pull request
ingest/filtering: enable filtering by default #4906
Closed
urvisavla
urvisavla reviewed Jul 13, 2023
View reviewed changes
Copy link
Contributor

@urvisavla urvisavla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks good to me. Would be good to get another set of 👀 to review it as well. Also, there are a few linter warnings, it'll be great if you could address those as well. Thanks!

urvisavla
urvisavla approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@urvisavla urvisavla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a changelog entry to reflect this change. Also, we need to update the public documentation for ingestion filtering with these modifications.

@sreuland
Copy link
Contributor

@aditya1702 , you can add Closes #4906 in the pr description, and then GH will automatically link the issue to this PR and also migrate the status of the issue to closed when the pr merges.

@aditya1702 aditya1702 changed the title Remove ingestion filtering flag Add functionality to hide flags from --help output Jul 19, 2023
@sreuland sreuland changed the title Add functionality to hide flags from --help output Add functionality to hide ingestion enabled flag from --help output Jul 20, 2023
@sreuland sreuland mentioned this pull request Jul 20, 2023
Merged
sreuland
sreuland approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, left a few minor comments on test coverage, thanks for evaluating the feedback.

@aditya1702 aditya1702 merged commit c83e4be into stellar:master Jul 24, 2023
24 checks passed
@aditya1702 aditya1702 deleted the remove-ingestion-filtering-flag branch July 28, 2023 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urvisavla urvisavla urvisavla approved these changes

@sreuland sreuland sreuland approved these changes

@mollykarcher mollykarcher Awaiting requested review from mollykarcher

Assignees

@aditya1702 aditya1702

Labels
horizon partner-experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ingest/filtering: enable filtering by default
3 participants
@aditya1702 @sreuland @urvisavla